Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added hoist field transform package + docs #2862

Merged
merged 10 commits into from
Mar 21, 2022

Conversation

ntziolis
Copy link
Contributor

@ntziolis ntziolis commented Sep 27, 2021

Description

Added transform that enables hoisting of fields. Its a thin wrapper around the HoistField transform from the @graphql-tools/wrap package.

Fixes # (issue)
This PR provides a transform that can be used as a workaround for issue ardatan/graphql-tools#3562

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Added relevant unit tests for the package.

Test Environment:

  • OS: Windows 10
  • @graphql-mesh/latest:
  • NodeJS: 14.x

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

Please let me know if there are any pieces missing to wire up the documentation with the website, since this this is the first time I'm adding an item vs just updatig.

We understand we could have just build our own package but felt this hoisting fields being a common across graphql-mesh users. If you feel this should be an independent package just let us know.

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2021

🦋 Changeset detected

Latest commit: c32c1be

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 83 packages
Name Type
@graphql-mesh/transform-hoist-field Minor
@graphql-mesh/types Patch
@graphql-mesh/website Patch
@graphql-mesh/cli Patch
@graphql-mesh/config Patch
@graphql-mesh/runtime Patch
@graphql-mesh/store Patch
@graphql-mesh/utils Patch
@graphql-mesh/cache-file Patch
@graphql-mesh/cache-inmemory-lru Patch
@graphql-mesh/cache-localforage Patch
@graphql-mesh/cache-redis Patch
@graphql-mesh/cache-store Patch
@graphql-mesh/graphql Patch
@graphql-mesh/grpc Patch
@graphql-mesh/json-schema Patch
@graphql-mesh/mongoose Patch
@graphql-mesh/mysql Patch
@graphql-mesh/neo4j Patch
@graphql-mesh/new-openapi Patch
@graphql-mesh/odata Patch
@graphql-mesh/openapi Patch
@graphql-mesh/postgraphile Patch
@graphql-mesh/raml Patch
@graphql-mesh/soap Patch
@graphql-mesh/thrift Patch
@graphql-mesh/tuql Patch
@graphql-mesh/transform-cache Patch
@graphql-mesh/transform-encapsulate Patch
@graphql-mesh/transform-extend Patch
@graphql-mesh/transform-federation Patch
@graphql-mesh/transform-filter-schema Patch
@graphql-mesh/transform-mock Patch
@graphql-mesh/transform-naming-convention Patch
@graphql-mesh/transform-prefix Patch
@graphql-mesh/transform-prune Patch
@graphql-mesh/transform-rate-limit Patch
@graphql-mesh/transform-rename Patch
@graphql-mesh/transform-replace-field Patch
@graphql-mesh/transform-resolvers-composition Patch
@graphql-mesh/transform-snapshot Patch
@graphql-mesh/transform-type-merging Patch
@graphql-mesh/merger-bare Patch
@graphql-mesh/merger-federation Patch
@graphql-mesh/merger-stitching Patch
@omnigraph/json-schema Patch
@graphql-mesh/container Patch
graphql-file-upload-example Patch
grpc-example Patch
grpc-reflection-example Patch
hasura-openbrewery-geodb Patch
json-schema-hello-world Patch
covid-mesh Patch
json-schema-example Patch
json-schema-fhir Patch
json-schema-subscriptions Patch
mongoose-example Patch
mysql-employees Patch
mysql-rfam Patch
neo4j-example Patch
nextjs-apollo-example Patch
odata-microsoft-graph-example Patch
odata-msgraph-programmatic-ts Patch
odata-msgraph-programmatic Patch
odata-trippin-example Patch
javascript-wiki Patch
typescript-location-weather-example Patch
openapi-react-weatherbit Patch
openapi-stackexchange Patch
openapi-stripe Patch
openapi-subscriptions Patch
openapi-youtrack Patch
postgres-geodb-example Patch
country-info-example Patch
soap-demo Patch
soap-netsuite Patch
chinook Patch
thrift-calculator Patch
type-merging-batching-example Patch
federation-gateway Patch
json-machete Patch
@omnigraph/openapi Patch
@omnigraph/raml Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ardatan
Copy link
Owner

ardatan commented Oct 5, 2021

Could this combine with this transform?
#2772
Maybe this covers wrap mode???
@santino @ntziolis

@santino
Copy link
Contributor

santino commented Oct 6, 2021

This is an interesting transform, but it can only deal with field hoisting, whilst my replace-field transform is primarily designed to override schema bits and so hoisting there is mostly a side conveniency feature.

I would personally avoid having two plugins that can achieve the same goal even if in different ways.

I think wrap mode can be implemented in replace-field transform, it just needs custom work across transformSchema and most importantly transformRequest and transformResponse.

I could start looking into that if we agree to go towards this path.

BTW I like here the idea of renaming the field, I will most likely add that to replace-field, it feels very relevant.
As for filtering args, it is convenient here, but in replace-field you can achieve it by defining a new GraphQL type with the args you want; this would override the base field you define as a source.

@ntziolis maybe you can have a look at what I did with replace-field and see if something like that would work for you.

@ardatan ardatan force-pushed the master branch 6 times, most recently from 4b3d5c5 to 8e8006d Compare October 6, 2021 10:59
@ntziolis
Copy link
Contributor Author

@santino Replace field seems significantly more powerful with the ability to hoist fields as well. Hence there is no need for a standalone hoist field transform anymore. Let me know if I can assist in creating a wrap mode version of your transform.

@santino
Copy link
Contributor

santino commented Oct 11, 2021

Glad to hear that can work for you.

Do you need wrap mode in order to start using the plugin?
I haven't started working on it yet.

If there is a need for that, as a feature request, then I will start looking into it ASAP; otherwise, I will leave it for when I have more spare time.

But keen to solve your use case, since you even created a PR for that.

@ntziolis
Copy link
Contributor Author

@santino We have been successfully using your transform. Here are some comments:

  • the transform is very powerful and enables A LOT of things and we are already heavily making use of it
  • it took us quite some time to understand how configuration done.
    • I think the documentation would benefit from a set of simple concrete use cases that don't build on each other
    • Which is not to say to not have one full blown example that pulls everything together as well
    • I'd be open to create a PR that builds on the existing documentation to assist others moving forward
  • it feels like from / to are backwards. Any chance this could be reversed? This actually cost us the majority of time.
  • one very powerful feature is the inline type + composer that can be used for intermediary purposes
    • it would be worth pointing out this use case more explicitly (that the type is not actually used in the end)
    • apart from that it would be great to specify type defs as part of a replacement in addition to globally all replacements
      • the list of replacements can get quite large
      • this way the
    • that said I also see use cases where a type def should span all replacements
  • it should be possible to replace to fields that do not exist yet right now one has to go the route via an intermediary type
  • retaining args when hoisting is often key use case when hoisting
    • any list call that supports pagination
    • but returns some funky / ugly return type that one just wants to clean up
    • without retaining the args that appear along the path this transform cant be used for this purpose

@santino
Copy link
Contributor

santino commented Oct 14, 2021

@ntziolis, your feedback is very relevant and I thank you for that!

I have some comments but I am thinking that is probably best to close this issue, so you can open a new one focused on the replace-field transform.
Most likely your comment above is a perfect conversation starter for the new issue.

Let's keep going on a dedicated thread :)

@ntziolis
Copy link
Contributor Author

Closing as discussed

@ntziolis
Copy link
Contributor Author

Reopening as discussed in #2975. Together with #3185 this transform is meant to replace the replace-fields transform.

@ntziolis ntziolis reopened this Nov 18, 2021
@ntziolis ntziolis force-pushed the hoist-field-transform branch from c18e20d to 76a4c50 Compare November 18, 2021 07:11
@ntziolis
Copy link
Contributor Author

Continuing discussion from here:

In this regard please note that I plan to extend the hoist transform to support glob patterns in the path Added hoist field transform package + docs #2862
this is to enable use cases e.g. where all list calls, return responses where the actual results are wrapped in some intermediary type
assuming the structure of these intermediary results is the same + its possible to create a glob pattern that matches all list calls it would then be possible to hoist the actual results and remove the intermediary type for all list calls at once

@santino For our use cases the support for glob pattern in the hoist field path would be immensely helpful. As of now the wrap transform leverages the HostFieldTransform from the graphql-tools which does not support patterns.
If we want to support glob patterns in the path. Should this be done in the mesh transform or in the downstream package? Since we would need the same targeting logic also for bare it might make sense todo this in the mesh transform, but I'm not certain how big the overlap would really be.

@ntziolis
Copy link
Contributor Author

ntziolis commented Nov 21, 2021

@santino The existing HoistField transform from the graphql-tools package, which is used for the wrap transform does not currently support lists in the path. I have created a bug report for this here.

When creating the bare version of hoist field we should ensure that lists in path are properly supported right from the start.

@ardatan ardatan force-pushed the hoist-field-transform branch from d4bf5fb to 42a138d Compare March 21, 2022 19:03
@vercel
Copy link

vercel bot commented Mar 21, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/theguild/graphql-mesh/6UoiEAeJ1fy6tJQVaM9pJrPeLcGy
✅ Preview: https://graphql-mesh-git-fork-ntziolis-hoist-field-transform-theguild.vercel.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants